Feature/load new lifecycle config#208
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
|
review from my side is finished. |
This removes the baselibs override
| { | ||
| public: | ||
| /// @brief Error codes returned when configuration loading fails. | ||
| enum class Error : std::uint32_t |
There was a problem hiding this comment.
Would it make sense to make just a ConfigError enum and not tie it into IConfigLoader
There was a problem hiding this comment.
I think this config loading will be the only place where this enum is used as this will be the only config loading code.
We can still move it out of the class and make if ConfigError though.
Do you see any benefit it moving it out of the class scope?
| private: | ||
| void rebuildPointers() const; | ||
| std::vector<EnvironmentVariable> entries_; | ||
| mutable std::vector<const char*> pointers_; |
There was a problem hiding this comment.
Since we only ever will need to get the char* const* of the env vars would it make sense to just figure out the pointers in the envp() method and not keep track of each pointer?
There was a problem hiding this comment.
The pointer returned by envp() is already assembled within the envp() method implementation and not updated whenever an environment variable is added. Most likely envp() is called a single time to get the pointer for passing it into execve system call.
Probably I do not understand your comment correctly.
There was a problem hiding this comment.
ah I see this is needed for the backing data when you return the const char* in envp. Guess we could also just construct the vector in envp() and return the vector so that we don't need this member and don't have to worry about reconstructing it after a move?
There was a problem hiding this comment.
The ides was to have it directly in the format required to be used with envp() systemcall.
If we return a vector the user of this will need to do the const_cast and .data() to bring it in the right format which seems a bit inconvenient.
The vector is build directly when calling envp() so I guess it will always the valid pointers, even if the object got moved before.
| } | ||
| } | ||
|
|
||
| ProcessState convertProcessState(fb::ProcessState fb_state) |
There was a problem hiding this comment.
Would it make sense to use something like this https://godbolt.org/z/TxqEW7dcE?
Don't really like how you need to hunt for the convertXYZ method, but up to you.
There was a problem hiding this comment.
I guess its a matter of preference. Whether you search based on the type or the name.
I tend to prefer the free functions because the name of the function makes clear what is being converted.
For example vector gids is :flatbuffers::Vector<uint32_t>* which based on the type is not really obvious.
Then there is also the fact that there are a variety of return types, some return the type directly others return expected<...> if conversion may potentially fail. Although also possible with the templates, it looks a bit more straightforward with the regular functions.
There was a problem hiding this comment.
The idea would be then that you that it's easy to separate each config type and write a converter for each type and the type definition in a single header. e.g. we have a config/details/environment.hpp that holds the Environment class definition and convert<fbsEnvironment, Environment>. In the end we can just include and use the function making it more readable.
I guess we can do the same with the convertXYZ methods?
Guess the root problem is that parseFlatbuffer is 100 lines which is pretty long for a function.
Right now I feel like this is similar to the old ConfigManager in that there is a huge piece of code that does all the converting and adding might be a pain.
There was a problem hiding this comment.
Or maybe even a static method on the type struct? e.g. Environment::FromFbs()
There was a problem hiding this comment.
How would having the templated convert methods simplify parseFlatbuffer?. Would it not just be calling convert<A, B>(...) instead of a free function for each conversion?
How about further dividing up into methods, like e.g. parseComponents and parseRunTargets?
I don't really like having the static methods as part of the types because I think the types should be independent of flatbuffer. If we were to change from flatbuffer to json at some point, all the types could stay unchanged.
The only change would be to implement a JsonConfigLoader and instantiate the JsonConfigLoader implementation in main instead of the FlatbufferConfigLoader.
71a4f93 to
8cd3a4b
Compare
Note: The code is not actually in use yet. The python script which does the translation from the user-facing json configuration to the flatbuffer json format is not part of the PR and will be added in a follow up PR.
Furthermore, the launch manager code needs to be adapted to use the new structure.
Background:
Relates To: #209